Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump to k8s 1.31 #664

Merged
merged 5 commits into from
Oct 15, 2024
Merged

Conversation

ArangoGutierrez
Copy link
Contributor

@ArangoGutierrez ArangoGutierrez commented Oct 15, 2024

I upgraded the K8s library versions to v1.31, and controller-tools to v0.16.4.

Fixes: #652

@ArangoGutierrez
Copy link
Contributor Author

Supersedes #655

@ArangoGutierrez ArangoGutierrez marked this pull request as ready for review October 15, 2024 15:15
Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Makefile Outdated
KUBECTL_VERSION=v1.30.4
ENVTEST_K8S_VERSION=1.30.0
KUBECTL_VERSION=v1.31.1
ENVTEST_K8S_VERSION=1.31.1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ENVTEST_K8S_VERSION=1.31.1
ENVTEST_K8S_VERSION=1.31.0

It seems that the latest version is v1.30.0:

setup-envtest list
(installed)  v1.31.0  darwin/arm64
(installed)  v1.30.0  darwin/arm64
(installed)  v1.29.3  darwin/arm64
(installed)  v1.29.1  darwin/arm64
(installed)  v1.28.3  darwin/arm64
(installed)  v1.27.1  darwin/arm64
(available)  v1.30.0  darwin/arm64
(available)  v1.29.3  darwin/arm64
(available)  v1.29.1  darwin/arm64
(available)  v1.29.0  darwin/arm64
(available)  v1.28.3  darwin/arm64
(available)  v1.28.0  darwin/arm64
(available)  v1.27.1  darwin/arm64
(available)  v1.26.1  darwin/arm64
(available)  v1.26.0  darwin/arm64
(available)  v1.25.0  darwin/arm64
(available)  v1.24.2  darwin/arm64
(available)  v1.24.1  darwin/arm64

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

go.mod Outdated
@@ -1,18 +1,18 @@
module github.com/kubeflow/mpi-operator

go 1.22.0
go 1.23.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
go 1.23.0
go 1.23

Could we eliminate the patch version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!
/lgtm
/approve

/hold for CI

@tenzen-y
Copy link
Member

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Oct 15, 2024
@tenzen-y tenzen-y mentioned this pull request Oct 15, 2024
7 tasks
@tenzen-y
Copy link
Member

tenzen-y commented Oct 15, 2024

/lgtm cancel

We need to upgrade the golangci-lint version.

@google-oss-prow google-oss-prow bot removed the lgtm label Oct 15, 2024
@tenzen-y
Copy link
Member

/approve cancel

@tenzen-y
Copy link
Member

I chat with @ArangoGutierrez offline.
He is trying to resolve CI errors now.

@@ -339,7 +339,7 @@ func NewMPIJobControllerWithClock(
priorityClassSynced: priorityClassSynced,
mpiJobLister: mpiJobInformer.Lister(),
mpiJobSynced: mpiJobInformer.Informer().HasSynced,
queue: workqueue.NewRateLimitingQueueWithConfig(workqueue.DefaultControllerRateLimiter(), workqueue.RateLimitingQueueConfig{Name: "MPIJobs"}),
queue: workqueue.NewTypedRateLimitingQueueWithConfig(workqueue.DefaultTypedControllerRateLimiter[any](), workqueue.TypedRateLimitingQueueConfig[any]{Name: "MPIJob"}),
Copy link
Member

@tenzen-y tenzen-y Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that we need to do this:

Suggested change
queue: workqueue.NewTypedRateLimitingQueueWithConfig(workqueue.DefaultTypedControllerRateLimiter[any](), workqueue.TypedRateLimitingQueueConfig[any]{Name: "MPIJob"}),
queue: workqueue.NewTypedRateLimitingQueueWithConfig(workqueue.DefaultTypedControllerRateLimiter[any](), workqueue.TypedRateLimitingQueueConfig[any]{Name: "MPIJob"}),

and then,

queue workqueue.RateLimitingInterface

workqueue.TypedRateLimitingInterface[any]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified my comment related to types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I was about to comment that any is required.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ArangoGutierrez Could you address the Line 250 as I mentioned above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did

Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
@tenzen-y
Copy link
Member

tenzen-y commented Oct 15, 2024

Additionally, I guess that we need to specify the 22.04:

@tenzen-y
Copy link
Member

It seems that the latest ubuntu runner has many issues: https://github.com/actions/runner-images/issues?q=is%3Aissue+Ubuntu+24.04+runner

@tenzen-y
Copy link
Member

Could you try to change the installation URL in the following?

curl -L -o $(PROJECT_DIR)/bin/kubectl https://storage.googleapis.com/kubernetes-release/release/${KUBECTL_VERSION}/bin/$(GOOS)/$(GOARCH)/kubectl

curl -L -o $(PROJECT_DIR)/bin/kubectl https://dl.k8s.io/release/${KUBECTL_VERSION}/bin/$(GOOS)/$(GOARCH)/kubectl 

@@ -61,7 +61,7 @@ const (
volcanoSchedulerManifestPath = rootPath + "/dep-manifests/volcano-scheduler/" // all in one yaml of volcano-development.yaml
envUseExistingSchedulerPlugins = "USE_EXISTING_SCHEDULER_PLUGINS"
envUseExistingVolcanoScheduler = "USE_EXISTING_VOLCANO_SCHEDULER"
defaultSchedulerPluginsVersion = "v0.29.8"
defaultSchedulerPluginsVersion = "v0.31.1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
defaultSchedulerPluginsVersion = "v0.31.1"
defaultSchedulerPluginsVersion = "v0.29.8"

This is the scheduler-plugins version, not k8s version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed back

Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!
This was not trival efforts!

/lgtm
/approve

@google-oss-prow google-oss-prow bot added the lgtm label Oct 15, 2024
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tenzen-y

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ArangoGutierrez
Copy link
Contributor Author

Thank you!

/lgtm

/approve

/hold for CI

Should we remove the hold?

@tenzen-y
Copy link
Member

Yup
/hold cancel

@google-oss-prow google-oss-prow bot merged commit a869150 into kubeflow:master Oct 15, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Next release date with updated k8s libraries for 1.31
2 participants